F1a: persist the runner lifecycle in the Store by mchwang · Pull Request #53 · codeabovelab/codeboost

mchwang · 2026-09-26T07:51:48Z

Lane F, step F1, slice F1a: the Store. The F1 contract (#49) is merged; this PR now targets main. Related: #22, #51.

What this does

Adds the durable half of the F1 runner lifecycle contract (docs/implementation/runner-lifecycle.md). There is no coordinator, HTTP or shutdown wiring yet; those are later slices.

Schema v6 and backfill. New tables tasks, attempts, user_actions and feedback_events. Every v5 plan gets one task row with the documented defaults. A merged plan becomes merged and gets its task-closed event. A null budget means "not started", never expired.
Guarded attempt transitions. Admission (including retry) checks the task status, the state version, the requeue claim, a pending cancel, an active attempt and the captured context in one transaction. recordFirstReason is the same-state "Stopping" transition, and markRunning is refused once a stop reason is recorded.
The Store chooses the terminal state. settleAttempt takes D's result and the in-memory first reason, and applies the precedence in runner/lifecycle.ts (classifySettlement), so the precedence lives in one place:
1. the saved or in-memory first reason (a D stop that came before shutdown wins);
2. then a changed context, which gives stale;
3. then D's own stop reason;
4. then the exit status and validation.
Two counters. Every plan-revision, snapshot or assignment change increases both the context generation and the state version in the same transaction (#savePlan, #snapshot, setAssignment). Lifecycle changes increase only the state version.
Task closure.
- cancelTask closes at once, or, with an active attempt, stops it first and closes when it settles. A pending cancel beats the time limit.
- Cancel is refused while a merge is being submitted or queued.
- finishMergeAttempt(merged) now closes the task and writes task-closed in the same transaction.
User actions. userAction gives exact replay for a UUID-v4 action ID: the same request returns the saved response without re-applying it. A different request with the same ID is refused. Guard refusals are recorded and replayed; storage errors are not.
Feedback events. recordFeedback works only inside a user action, and supersedes by source. feedbackEvents is lane J's read path and is available once the task closes.
Nested transactions. #transaction now joins an outer transaction, so a user action can wrap existing Store methods atomically.

Contract change found while implementing

The running → failed guard in the contract said "no first reason". That contradicted the round-19 rule that a D stop before shutdown ends failed. I fixed the row on #49 (e44d04a).

Merge dependency: #51

#51 says the F1 implementation "must not merge until items 1–5 land". Those items are lane D changes (bounded settlement, asynchronous launch, resource labels, scoped recovery, abortable preparation), and #51 is still open. This slice changes only the Store and the merge path; it doesn't call those D APIs. But the gate is recorded, so merging waits for #51 unless the lane owner decides this slice is exempt.

Changes since the last description (merged `main`, then 21 Codex review passes)

Merged main at b3da1b3 (no conflicts).
Merge admission is bound to the task: it refuses a closed task, a pending cancel, a non-review status, an active attempt, or a task state version that changed after the click. MergeCoordinator passes the version it saw when the request arrived. Merge readiness shows a task blocker for the same statuses.
Runner work can't start while a merge is submitting or queued, and a closed task can't be reassigned.
Admission enforces the whole-task budget: an expired task moves to needs human and admission is refused. That move survives the refusal, including inside userAction (RefusalWithEffect).
recordFeedback must use the action ID of the enclosing userAction.
Merge clicks are idempotent end to end:
- the browser sends a UUID actionId; the server refuses a merge without one (400);
- the coordinator joins a resend of the in-flight click, and otherwise replays a saved outcome before any other guard;
- the attempt and its saved response commit together, and the saved response follows the attempt's state;
- definite refusals are saved, while deadline, shutdown and pre-admission aborts answer 503 and save nothing;
- failures replay as failures, a direct merge keeps its URL, and a failing reload doesn't hide a committed merge;
- the browser keeps its key until a parsed answer resolves the click, and drops it once that click's attempt ends.
AGENTS.md gains two rules from this review (see the audit).

Validation

Head b5bdff5 was the validated head for Codex pass 21 (no defects). Copilot round-1 fixes on bb35e43: typecheck, 583 unit and 62 browser tests passed. Round-2 fixes on acb7b4d: typecheck, 585 unit and 62 browser tests passed (one history.test.ts timeout under load passed twice on rerun).

At b5bdff5:

npm run typecheck: passed.
CI's unit set (npm test minus the Docker suites): 582 passed.
npm run test:browser: 62 passed.
GitHub CI: passed.
Every fix above has a regression that fails with that fix removed (checked by mutation), except one defensive guard that can't be reached today: admission refusing an active merge. Admission needs queued or running, and a merge needs a review status.
The Docker suites weren't run; this PR doesn't touch agents/.

Review-lesson audit (Codex passes 1–21)

Finding (pass, severity)	Classification
Merge outcome didn't refresh the saved replay (1, P2); production merge bypassed replay (4, P2); resend hit the active guard (5, P2); pre-admission refusals unsaved (5, P2); direct merge URL lost (5, P2); `actionId` optional (7, P2); failed merge replayed as success (8, P2); replay after shutdown (11, P2); resend before the action was saved (12, P1); direct resend got interim state (13, P2); retry reused a resolved key (14, P2); deadline dropped the key (15, P2); unreadable body dropped the key (16, P2); pre-admission-only 503 (18, P1); concurrent refusal unsaved (18, P2); concurrent coordinators (19, P2); unsaved refusal reported as definite (20, P1)	New rule in this branch: AGENTS.md, "Make an idempotency key required at the API boundary…"
Budget handoff rolled back by its refusal (8, P1)	New rule in this branch: AGENTS.md, "When a refusal must also change durable state…"
Merge not bound to task state (2, P1)	Covered: AGENTS.md "After the final asynchronous external validation, re-read the local generation…"
Merge during an active attempt (3, P1); runner work during a merge (11, P2); readiness ignored task status (14, P2)	Covered: AGENTS.md "Validate coupled lifecycle fields as allowed combinations."
Committed merge hidden by a failing reload (9, P2)	Covered: AGENTS.md "Once an irreversible external command succeeds, do not convert later refresh or rendering failures into action failure."
Event not bound to its action (3, P2)	One-off: a Store API precondition from runner-lifecycle.md ("Feedback-event contract"), not a general pattern.
Budget not enforced at admission (6, P2); closed task reassigned (17, P2)	One-off: missing checks of rules already stated in runner-lifecycle.md.
Review feedback not routed into events (14, P1)	Deferred to F1e (#60), which owns that wiring.

Copilot review (round 1, fixed in bb35e43):

Finding	Classification
A D stop reason with exit 0 settled as completed	One-off: missed row in runner-lifecycle.md's settlement table ("a missing stopReason means the agent finished normally").
Reassignment allowed during an active merge	Covered: AGENTS.md "Validate coupled lifecycle fields as allowed combinations."
Merge-attempt changes didn't bump the state version	One-off: missed application of runner-lifecycle.md's "State version" definition.
Budget handoff window between transactions	Not a defect (replied): every admission re-checks the unmovable deadline, and the effect re-checks its preconditions.
Failed replay lacks structured fields	Not a defect (replied): the replay returns the same `{error}` as the first response.

Copilot review (round 2, fixed in acb7b4d):

Finding	Classification
Plan edits allowed during a merge; task-closed used the current context	Covered: AGENTS.md "Validate coupled lifecycle fields as allowed combinations."
Reconciled direct merge lost its URL for replay	Covered: this branch's idempotency rule ("…replay failures as failures with complete result fields").
Browser dropped the key after an admitted merge with an unknown outcome	Covered: this branch's idempotency rule, plus AGENTS.md "When an irreversible command has an ambiguous … outcome, retain durable in-flight ownership".

Copilot review (round 3, 8ba3e65):

Finding	Classification
Busy/locked SQLite saved as a refusal	Not a defect (replied): node:sqlite reports it as `ERR_SQLITE_ERROR`; a real-lock regression now pins that.
Success returned when the post-merge local write fails	Not a defect (replied): required by AGENTS.md's committed-result rule; the attempt stays durably `submitting` and reconciles.
Browser clears the key on a 200 for a queued attempt	Not a defect (replied): a 200 is definite, and the button stays disabled until the attempt ends.

Copilot review (round 4, fixed in 3498b3f):

Finding	Classification
Plan writes and HEAD snapshots could change a closed task	Covered: runner-lifecycle.md "a closed status never changes again" and AGENTS.md "Validate coupled lifecycle fields as allowed combinations."

Copilot review (round 5, summary only, fixed in 83451f3):

Finding	Classification
A storage error while reading a saved outcome answered 409, so the browser dropped its key	Covered: this branch's idempotency rule ("Only a passing, nothing-applied outcome (shutdown, abort, deadline, storage error) stays resendable").

Not in this slice

Review feedback into events (Codex pass 14, P1): recordFeedback has no production caller yet. Routing assignments, acceptances and change notes through userAction is F1e's scope (F1e: planning API for lane G and feedback from review actions #60).
F1b (coordinator), F1c (shutdown wiring and /api/runner), F1d (startup recovery and the OS lock), F1e (planning endpoints and wiring feedback into review actions). The F1 implementation must not merge as a whole until D follow-ups required by the F1 runner lifecycle contract #51's pre-F1 D items land.

🤖 Generated with Claude Code

Schema v6 with a v5 backfill: tasks, attempts, user_actions and feedback_events. Guarded attempt transitions, where the Store chooses the terminal state from the first reason, D's result and context currency. State version and context generation counters, cancel task, merge closure, replayable user actions (including refusals) and feedback events. Implements the Store slice of docs/implementation/runner-lifecycle.md. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Merge main. Record the F1 lifecycle contract (#49) and open F1a-F1c (#53, #56, #57); record the ranked Issues screen (H4a, #55) with H4b's trust action remaining; Issues is now a menu link, not a placeholder. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* docs: reconcile the design plan with the code Record where the code differs from the approved plan and update stale status: - Record Ask as an interim exception to R1: it runs the vendor CLI on the host with tools off until lane F moves it into the lane D container. - Amend D20: there is no "Merge anyway"; to override a blocker, merge on GitHub. Matches docs/implementation/guarded-merge.md. - Tick T1, T2, T4, T5, T10, T13, T14 with test evidence; point Files lines at core/linking.ts and core/approvals.ts instead of never-created modules. - Mark increment 1 merged; add a lane status table (C, D, K done; E, F, H progress); record decided open questions (issue ranking, AgentDiff). - Add a verified status note for design tasks DT2-DT15; none newly ticked. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs: note merge-queue support in the guarded merge doc The guarded merge gate doc still said merge-queue branches stay blocked. #46 (closing #24) added queue lifecycle support. Point to merge-queue.md, and state that adapters without queue inspection still fail closed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs: bring plan status up to date with F1 and H4a Merge main. Record the F1 lifecycle contract (#49) and open F1a-F1c (#53, #56, #57); record the ranked Issues screen (H4a, #55) with H4b's trust action remaining; Issues is now a menu link, not a placeholder. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs: add F1d and Ask PRs to lane status Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Align merge-queue wording with the merged K2/K3 support README no longer says merge queues block merging; it describes the enqueue-then-confirm behaviour. The plan's wave-3 note records the old block as history instead of a live instruction. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Record the K-lane queue block as history in the task table Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * README: disclose that Ask runs the agent CLI on the host The plan (R1 exception) says README states this limit; it did not. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Add F1e (#60) and the #51 merge condition to the F lane row Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * README: distinguish queue-removal retry from changed-head review Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Plan: mark the install preflight and npx entry as planned The CLI checks only the Node version today; say so instead of describing the git/gh/container/sign-in preflight as current. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

* docs: reconcile the design plan with the code Record where the code differs from the approved plan and update stale status: - Record Ask as an interim exception to R1: it runs the vendor CLI on the host with tools off until lane F moves it into the lane D container. - Amend D20: there is no "Merge anyway"; to override a blocker, merge on GitHub. Matches docs/implementation/guarded-merge.md. - Tick T1, T2, T4, T5, T10, T13, T14 with test evidence; point Files lines at core/linking.ts and core/approvals.ts instead of never-created modules. - Mark increment 1 merged; add a lane status table (C, D, K done; E, F, H progress); record decided open questions (issue ranking, AgentDiff). - Add a verified status note for design tasks DT2-DT15; none newly ticked. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs: note merge-queue support in the guarded merge doc The guarded merge gate doc still said merge-queue branches stay blocked. #46 (closing #24) added queue lifecycle support. Point to merge-queue.md, and state that adapters without queue inspection still fail closed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Run Ask in the lane D agent container Ask used to run the claude/codex CLI on the host with each CLI's own restrictions, an interim exception to R1. It now uses lane D's invocation boundary in the read-only "questions" phase: a clone of the reviewed snapshot head at /work, no commands, vendor-only network, and no other host files. There is no host fallback. - runner/question-container.ts: build image, clone, allocate bounded storage, capture, start the Claude/Codex adapter; release storage only after the invocation settles. Deps are injectable for unit tests. - runner/question-worker.ts: lane D setup is synchronous, so a worker thread owns it and the review server stays responsive. - runner/question-agent.ts: QuestionWorker bridge; a question settles only when the worker reports the container and storage are gone. - Credentials come from the environment only: CLAUDE_CODE_OAUTH_TOKEN for Claude, CODEBOOST_CODEX_AUTH_FILE or CODEX_HOME/auth.json for Codex. - Provider failures include the vendor's short message (e.g. a 401). - test/agent-question.test.ts runs the path on real Docker (Agent isolation workflow); its live case needs the auth-probe credentials. - Plan, README, Settings copy and implementation docs updated; the R1 exception is closed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs: bring plan status up to date with F1 and H4a Merge main. Record the F1 lifecycle contract (#49) and open F1a-F1c (#53, #56, #57); record the ranked Issues screen (H4a, #55) with H4b's trust action remaining; Issues is now a menu link, not a placeholder. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs: add F1d and Ask PRs to lane status Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Align merge-queue wording with the merged K2/K3 support README no longer says merge queues block merging; it describes the enqueue-then-confirm behaviour. The plan's wave-3 note records the old block as history instead of a live instruction. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Record the K-lane queue block as history in the task table Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * README: disclose that Ask runs the agent CLI on the host The plan (R1 exception) says README states this limit; it did not. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Add F1e (#60) and the #51 merge condition to the F lane row Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Bind Ask answers to their attempt and keep cleanup ownership - Reuse the persisted answer attempt as the invocation attempt, and the note's contextId as referencedCodeHash. Accept a result only when its attempt and context match the captured invocation and the worker reply carries the same attempt. - Treat a missing exit code or any signal as a failure, not an answer. - Keep task storage whose removal Docker did not confirm, retry removal before the next question, and refuse Ask while any remains. - After a worker crash, fail closed instead of starting a replacement: its containers and storage may still exist, and reclaiming them needs lane D's scoped recovery (#51 item 4). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Run the Docker Ask suite when runner/questions.ts changes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Record Ask storage left at shutdown and keep Ask off until it is gone Terminating the question worker dropped its only handles to storage that Docker had not removed. Shutdown now asks the worker for one last bounded removal, records anything still unremoved beside the review database, and the next session refuses Ask, with the removal commands, while any recorded container or volume still exists. The record clears itself once they are gone; an unreadable record or unreachable daemon keeps Ask off. Removal through D waits for its recovery handles (#51). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Keep Ask off after a setup failure that leaves unidentifiable storage When task storage setup fails and lane D cannot confirm its own cleanup, D returns no handle, so Ask cannot name the leftovers. Ask now counts the failure, stays off for the session, records it at shutdown, and after a restart stays off while any io.codeboost.task-storage container or volume exists. Caller-provided allocation IDs (#51 item 3) would let Ask name these resources instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Make the Ask leftover gate bounded and fail closed on unknown state - A worker crash, or no release report at shutdown, is recorded at once as unidentified leftovers instead of an empty, clean release. - The pre-question check is two label queries (docker ps, docker volume ls) under one 15-second limit that the question's signal can cancel, instead of up to 300 sequential inspects. - Entries beyond the record's cap become unidentified leftovers; none are dropped. - Removal commands list only resources that still exist, so a missing keeper no longer blocks volume removal. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Bound Ask against unsettled lane D cleanup and scan all D labels - Scan containers, volumes and networks for every label lane D applies (allocation, invocation, egress), so a leftover seeder or proxy keeps Ask off. - The first question of each process scans even without a record, so a process killed before writing one cannot bypass the gate. - A question not settled 30 s after its deadline, or still settling after a 20 s shutdown grace, abandons the worker: unknown leftovers are recorded, waiters rejected and the worker stopped, so D's unbounded cleanup retries (#51 item 1) cannot hang Ask or shutdown. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Own the host staging directory like the Docker allocation If the host copy of the reviewed code cannot be deleted, the worker now keeps its path and retries before the next question, shutdown records it, and the next leftover check deletes it. Ask stays off while any copy remains. The record accepts only codeboost-question-* staging paths. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Keep credentials out of setup subprocesses and settle abandon in order - The question worker snapshots credentials for the adapters and removes credential-like variables from its own environment, so the image build, clone and other setup subprocesses cannot inherit them. Leftover Docker queries use lane D's minimal PATH/DOCKER_HOST environment. - Missing sign-in is reported before the leftover scan or any Docker work. - Abandoning a worker records unknown leftovers, then waits (bounded) for the thread to stop before rejecting its questions, so their slots stay owned until a synchronous Docker or Git call has returned. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Own every host copy through a recorded Ask root; fix CI env dependency - The bridge creates one Ask root per worker (<tmp>/codeboost-ask-*), records it before the worker starts, and runs the worker with it as TMPDIR, so the reviewed clone, lane D's input directory and its Codex auth copy all live inside it. The root is deleted after the thread stops (clean shutdown, crash or abandon); otherwise the next check deletes it, and Ask stays off while an earlier root remains. - The record accepts only direct children of the real temp directory named codeboost-ask-XXXXXX, so a lookalike path elsewhere is refused instead of deleted. - Test fix: the bridge checks sign-in before asking, so the stub worker now gets its own Codex auth file instead of depending on ~/.codex. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Delete an abandoned worker's root once its thread finally stops If the bounded wait for an abandoned worker ends while its thread is still inside a synchronous Docker or Git call, its ownership is already durable (unknown leftovers and the recorded Ask root) and no new question is admitted. The root is now also deleted, and dropped from the record, as soon as that thread does stop. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Serialize Ask per review with a lock and bound the release timeout - Take an exclusive per-review Ask lock (PID lock file next to the leftover record) before the startup scan and hold it until the worker has stopped; only the holder scans, starts a worker or writes the record. A lock left by a dead process is taken over. - A worker that does not answer the final release request now goes through the bounded abandon path instead of an unbounded terminate, keeping its root and the lock until the thread stops. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Test that a refused second process cannot delete a live Ask root Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Give the Ask worker an allowlisted environment; credentials as data Replace the name-based credential scrub with an explicit allowlist: the worker's environment is only PATH, DOCKER_HOST and its Ask root as TMPDIR, so every setup subprocess (including the image build) inherits no credentials, home directory, Docker config or agent socket. The credential lookup's four variables reach the worker via workerData and go only to the adapters. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Use an OS lock for Ask and key it by the canonical database path - Replace the PID file and liveness takeover with an exclusive SQLite transaction on the lock file: an OS file lock the operating system releases when its process ends, so PID reuse cannot let two holders overlap and no takeover is needed. - Key the lock and leftover record by the database's realpath, so relative, absolute and symlinked spellings share them; refuse Ask on a database with other hard links. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Clean up the Ask root when the worker cannot be constructed If the Worker constructor throws after the root was created and recorded, delete the root and drop it from the record, so close() can release the per-review Ask lock. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Serialize Ask worker abandonment and bound the host clone up front - All abandon triggers (crash, watchdogs, shutdown) share one bounded termination promise, so a second trigger cannot reject questions and free their slots while the thread is still in a synchronous call. - Before lane D's unbounded host clone, measure the checkout at the reviewed head and the object store with Git plumbing and refuse a repository that would not fit the question's storage allocation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Never drop recorded Ask roots; keep the marker for unnamed resources - Ask roots are never sliced from the record; recording one past the cap is refused, which also refuses to start another worker. - Any labelled Docker resource that is not part of a still-listed allocation (a seeder, agent container, proxy or network) keeps the unidentified marker after the named entries are gone, and it clears only when none remain. Named removal commands are reported first. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Make the Ask startup scan single-flight Concurrent first questions now share one startup scan instead of each running their own, so a second scan cannot see the first question's new labelled resources and record them as earlier-session leftovers. Each caller can stop waiting through its own signal, and a failed scan is retried by the next question. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Wait for an in-progress abandonment when the Ask worker closes If a crash or watchdog is already abandoning the worker when shutdown calls close(), close() now awaits that bounded settlement instead of returning at once, so the thread, its recorded root and the lock are settled before Questions.close() finishes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Key the Ask lock by file identity; finish the startup scan under it - The lock file is keyed by the database's device and inode in the temp directory, so every spelling and every later name of the file, including an atomic rename while a server runs, finds the same lock. The durable record stays next to the canonical database path. - close() waits for a shared startup scan still in flight before releasing the lock, so the scan cannot write the record unlocked. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Find Ask roots by their owner stamp, not only through the record The durable record sits beside the database path, so after a rename a new process would not see roots recorded under the old name. Each Ask root now carries an .owner stamp naming its lock, written under a preparation name before the folder is renamed into place. The first check of a process deletes unrecorded codeboost-ask-* folders whose owner lock is free and leaves those whose owner is still running. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Harden the Git size check and trust only codeboost lock stamps - The pre-clone Git measurement now uses the same hardening as lane D's clone: GIT_NO_LAZY_FETCH, protocol.allow=never, no replace objects, no hooks, no graft file, no submodule recursion. - An .owner stamp is probed only when it names a codeboost lock file in the temp directory; anything else counts as no owner, so a lookalike root cannot make startup open or create a file elsewhere. Every ledger's lock now lives there under that name. - Regression test: startup still scans Docker after deleting a recorded root (the reported bypass does not reproduce). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Name the labels the Ask leftover scan actually checks The docs and one Ask error message still said io.codeboost.task-storage, but the scan checks containers, volumes and networks labelled io.codeboost.allocation, io.codeboost.invocation or io.codeboost.egress. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Write the Ask leftover record through an exclusive random temp file The record was written via a predictable <record>.<pid>.tmp name with the default "w" flag, so a planted link at that name would be followed and its target overwritten. Use a random name opened with "wx" and delete it if the write or rename fails. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Stop Ask admission when shutdown begins; keep the root if recording fails - The server stops question admission in the same turn it starts shutting down, so a request still arriving cannot start an agent or a container worker during the drain. A question it saved gets a retryable "Server stopped" answer without any agent starting; the existing drain test now expects zero agent calls instead of one started-then-cancelled call. - If the final release report cannot be saved, the worker's root is no longer deleted: it stays on disk and in the record for the next session, and Docker leftovers remain covered by the startup scan. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Release the Ask lock when the final record write fails After a failed release-report write the worker thread has stopped and the root is already recorded, so let go of the root in this process: it stays on disk and in the record for the next check, and close() can release the per-review lock instead of holding it for the process. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Treat a failed worker termination as not stopped A rejected terminate() no longer counts as a stopped thread: the Ask root stays on disk and in the record, and the lock stays held, instead of being removed while the worker may still be alive. Later cleanup runs only after a termination that actually settled. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Add AGENTS.md rules from the PR #54 review-lesson audit Four rules for owned host and Docker resources: durable cleanup ownership, allowlisted subprocess environments and credential channels, untrusted record and on-disk paths, and cross-process OS locks keyed by stable identity. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Close the store on failed Ask cleanup; private locks; keep lock after abandon - web/server.ts closes the review store in a finally block when Ask's cleanup fails, and the CLI exits non-zero instead of hanging. - Ask lock files live in a private per-user directory under the temp directory (mode 0700, checked ownership); a lock path that is a symlink or not a plain file is refused, never opened. - After any abandonment the review lock is kept until the process exits: Docker CLI children the terminated thread started can outlive it and cannot be awaited until lane D exposes process groups (#51). - AGENTS.md: the untrusted-path rule also forbids following links and requires a private directory for plantable files. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Pass the Ask stop reason as a typed value; leave foreign Ask folders alone - Closes #64: the stop reason (timeout, shutdown, cancelled) now travels as a StopError value from Questions through the worker message to handle.cancel(), instead of being rebuilt from message wording. - The orphan-root scan deletes only folders this user owns that carry a valid createAskRoot stamp naming a lock in the private lock directory whose owner is gone; unstamped, tampered or foreign folders stay. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Describe which Ask folders the orphan scan deletes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Read the Ask record and owner stamps without following links The leftover record and each folder's .owner stamp are now read through O_NOFOLLOW and accepted only as regular, single-link files within a size limit. A linked record makes the ledger unreadable (Ask fails closed and never acts on the record it points to); a linked stamp leaves the folder alone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

Codex review of #53: finishMergeAttempt updated only merge_attempts, so a merge click resent after a lost response replayed the saved "in progress" answer (runner-lifecycle.md: the outcome transaction updates the saved response). beginMergeAttempt now takes the starting action ID, and every attempt change refreshes that user_actions row in the same transaction with mergeActionResponse(attempt). Regression fails without the refresh. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Codex review of #53 (P1): beginMergeAttempt checked only the review state, so a stale merge click after Cancel task could start a merge, and its confirmation would reopen the cancelled task as merged with a second task-closed event. The admission transaction now refuses a closed task, a pending cancel, and (when given) a task state version other than the expected one. MergeCoordinator captures the task state version when the merge request arrives and passes it after the final await, as runner-lifecycle.md "Irreversible actions" requires. Regressions fail without each guard. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…ction Codex review of #53, pass 3: - P1: a merge could start while an attempt was pending or running, because the captured task version already included it. Merge admission now requires MERGEABLE_STATUSES ('in review', 'approved but merge blocked') and no active attempt, in the admission transaction. - P2: recordFeedback checked only that some transaction was open, so a callback could commit an event under another action ID with no user_actions row. userAction now records the running action, and recordFeedback requires the same plan key and action ID. Regressions fail without each guard. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Codex review of #53, pass 4 (P2): the Store could replay a merge click, but the production path passed no action ID, so a merge resent after a lost response failed as "already active" instead of reporting the outcome. - The browser sends a UUID actionId per merge click and keeps it until a definite response arrives (a TypeError from fetch means none did). - The server passes it to MergeCoordinator.merge(token, actionId). - The coordinator replays a saved action before any validation (using the cached display status), and otherwise begins the attempt inside Store.userAction, so the attempt and its saved response commit together. - Store.savedAction is the shared replay lookup, factored out of userAction. Regression fails without the replay. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…URLs Codex review of #53, pass 5 (three P2s in the merge replay path): - A resend that arrived while the first merge was still running hit the "already running" guard before the replay lookup. merge() now replays a saved click first. - A refusal before admission (blocked checks, stale token, changed requirements) was not saved, so reusing the action ID later could merge. Definite pre-admission refusals are now recorded through userAction; aborts, the deadline and shutdown stay resendable. - A direct merge did not store its URL, so a replay returned an empty one. finishMergeAttempt accepts the URL and the direct path passes it. Each regression fails without its fix. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Codex review of #53, pass 6 (P2): admitAttempt checked only the attempt deadline, so after budget_deadline passed a retry or fresh attempt could still start. Admission now moves an expired task to 'needs human' (the time-limit mapping in runner-lifecycle.md) in a committed transaction and refuses. The existing "keeps the budget across attempts" test retried after its budget expired, so it now retries inside the budget. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Codex review of #53, pass 7 (P2): the server accepted a merge without an actionId, which bypassed replay, so a refused click resent after a state change could merge. runner-lifecycle.md "User actions" requires the server to refuse a missing or invalid key with HTTP 400 before any work. The merge route now does; the two browser tests that post merges send a key, and a new browser test checks the 400 and that nothing was stored. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

… errors Codex review of #53, pass 8: - P1: inside userAction (the retry path), admitAttempt's move to 'needs human' was written in the outer transaction and rolled back by its own refusal, leaving the task running. The refusal is now a RefusalWithEffect: userAction commits its effect with the saved refusal, and a direct call commits it in its own transaction. The effect re-checks that the task is still open, idle and past its budget. - P2: a failed or removed merge replayed as a 200 "submitted" with an empty URL. Replay now rejects with the saved reason, like the first response. Both regressions fail without their fixes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Codex review of #53, pass 9 (P2): replay called service.load() outside the displayStatus fallback, so a failing local refresh reported an already merged click as blocked (409) instead of its saved URL (AGENTS.md: never convert a committed irreversible result into action failure). A failing reload or status read now yields an unavailable "refresh to confirm" status alongside the saved result. Regression fails without the fix. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Ten Codex passes on #53 found gaps in merge replay (required key, lookup before guards, saving pre-admission refusals, current saved responses, failures replayed as failures, complete result fields) and a refusal whose state change was rolled back. Two rules capture both patterns. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Codex review of #53, pass 11 (two P2s): - transitionTask and admitAttempt ignored an in-flight merge attempt, so runner work could start while GitHub might still merge the reviewed head. Both now refuse while the latest merge is submitting or queued. - A resend of a saved merge after close() began was refused as "shutting down" before replay. The coordinator now replays first. The server's HTTP 503 during shutdown stays (runner-lifecycle.md), and the browser now keeps the action key on a 503 as well as on a lost response, so the next click resends it. The AGENTS.md rule names that one exception. Regressions (store, coordinator, browser) fail without their fixes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Codex review of #53, pass 12 (P1): a resend that arrived while the first click was still validating, before its action was saved, missed replay and got "already running"; the browser dropped the key while the original could still merge. The coordinator now remembers the active click's key and request, returns the same promise to a matching resend, and refuses the same key with a different request as a reused ID. Regression fails without it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Codex review of #53, pass 13 (P2): for a direct merge still running, a resend returned the saved "submitting" state instead of joining the active merge; direct attempts are not polled, so the UI stayed on "Submitting". The coordinator now joins a matching active click first and replays only when no merge for that click is in progress. The regression now asserts the resend gets the final result, and fails with the old order. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Codex review of #53, pass 14 (two P2s; the P1 to route review feedback through userAction is F1e's scope, #60, and stays out of this slice): - Merge readiness ignored task status, so Merge PR could render ready for a queued, running or human-gated task. Status now adds a 'task' blocker unless the task is in a mergeable status. - After a lost response the browser kept its key, and once that attempt ended, Retry resent it and only replayed the old failure. Merge status now exposes the attempt's actionId, and the browser drops its retained key when the attempt that key started is merged, removed or failed. Both regressions fail without their fixes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Codex review of #53, pass 15 (P2): a deadline abort left the action unsaved on purpose, but the server answered 409 and the browser dropped the key, so the next click was a new action instead of a resend. The coordinator now raises MergeNotApplied for an abort (deadline or shutdown) and for a coordinator that is shutting down, and the server maps it to 503; the browser already keeps its key on 503. Coordinator and server regressions fail without the change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Codex review of #53, pass 16 (P2): an empty or truncated JSON body made response.json() throw a SyntaxError with no status, and the browser dropped the key although the outcome was unknown. The browser now drops the key only for a parsed server answer with a definite status (anything but 503); no response, an unreadable body, or a 503 keeps it. The browser regression adds the unreadable-body case and fails with the old rule. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Codex review of #53, pass 18: - P1: an abort during the GitHub merge command raised MergeNotApplied (503, "nothing applied") although the attempt was durable and its outcome unknown. Only an abort before admission is MergeNotApplied now; after admission the original error is reported and the attempt stays in flight to reconcile, as merge-queue.md documents. - P2: "A merge attempt is already running" was not saved under the second click's key, so a resend after a lost 409 could start a merge later. It is now recorded like other pre-admission refusals (shared #recordRefusal). Both regressions fail without their fixes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Codex review of #53, pass 19 (P2): two coordinators sharing a database could both validate the same actionId; the second missed the first replay lookup and answered 409 instead of the saved outcome. When validation or admission refuses a click that now has a saved outcome, the coordinator returns that outcome instead of the refusal. Two regressions (save during validation, save just before admission) fail without it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Codex review of #53, pass 20 (P1): #recordRefusal swallowed every error, so a busy or failed database left a pre-admission refusal unsaved while the server still answered a definite 409; a resend after a lost response could then be evaluated as a new merge. Only the re-raised refusal and an already saved outcome are treated as settled now; any other failure becomes MergeNotApplied (503), so the browser keeps its key. Regression fails without it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate lifecycle and merge correctness issues remain.

Review effort: Lite
Findings: 3 · 2

Open (5)

Treat all non-first stop reasons as unsuccessful · New
Block assignment changes during active merges · New
Serialize expired-budget refusal with expiry handoff · New
Preserve structured failure results when replaying saved merges · New
Increment state version for merge attempt mutations · New

What changed in this PR

Adds durable runner lifecycle storage and idempotent merge actions across the server, coordinator, browser, and tests.

Changes:

Adds schema v6 lifecycle persistence, guarded transitions, budgets, cancellation, and feedback events.
Implements merge replay and browser idempotency handling.
Expands lifecycle, merge, and browser regression coverage.

File	Reviewed changes
`web/server.ts`	Merge action validation and retry responses
`web/public/app.js`	Client-side merge idempotency keys
`test/runner-lifecycle-store.test.ts`	Store lifecycle coverage
`test/merge.test.ts`	Merge coordination coverage
`test/browser/review.spec.ts`	Browser retry coverage
`runner/store.ts`	Durable lifecycle persistence and transactional guards
`runner/merge.ts`	Idempotent merge coordination and replay
`runner/lifecycle.ts`	Settlement classification and lifecycle types
`AGENTS.md`	Lifecycle and idempotency rules

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot review of #53: - classifySettlement published 'completed' for a D stop reason such as cancelled or shutdown with exit 0 and no first reason. Any D stop reason now ends 'failed' (a missing stopReason is the only normal finish). - setAssignment could change the context while a merge was submitting or queued; it now refuses during an active merge, like transitionTask. - Merge-attempt inserts and changes did not bump the task state version. They now do, in the same transaction, without changing the context generation (runner-lifecycle.md "State version"). Each regression fails without its fix. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot

Copilot review overview

🔵 Needs a closer look

Seven moderate issues remain unresolved across lifecycle, merge, server, browser, and reconciliation paths.

Review effort: Lite
Findings: None

Resolved since last review (5)

Serialize expired-budget refusal with expiry handoff
Block assignment changes during active merges
Treat all non-first stop reasons as unsuccessful
Increment state version for merge attempt mutations
Preserve structured failure results when replaying saved merges

Copilot

Copilot review overview

🟡 Changes recommended

Unresolved lifecycle consistency and merge idempotency issues remain, including a critical context-change race during active merges.

Review effort: Lite
Findings: 1 · 2

Open (3)

Context changes can invalidate active merge attempts · New
Reconciliation loses direct merge URL · New
Client drops key for nonterminal merge attempts · New

Copilot review of #53 (round 2): - Plan edits (importRevision, applySuggestion) are refused while a merge is submitting or queued, and a confirmed merge closes the task with the merged attempt's revision and snapshot. HEAD observation (recordHistory) stays allowed: the merge is pinned to the reviewed head. - A direct merge reconciled after a lost response now records GitHub's PR URL (read with `gh pr view --json url`), so a replayed click reports it. - After admission, an unknown GitHub outcome raises MergeOutcomeUnknown; the server answers 409 with outcomeUnknown, and the browser keeps its key until that attempt ends. Only GitHub's confirmed refusal stays definite. Each regression fails without its fix. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate durability, replay, validation, and task-closure issues remain.

Review effort: Lite
Findings: 3

Open (3)

Swallowed outcome write leaves merge attempt stuck · New
SQLite storage errors are misclassified as refusals · New
Idempotency key cleared before attempt reaches terminal state · New

Resolved since last review (3)

Context changes can invalidate active merge attempts
Client drops key for nonterminal merge attempts
Reconciliation loses direct merge URL

Copilot review of #53 (round 3) asked whether busy or locked SQLite errors escape userAction's storage check. node:sqlite reports them as ERR_SQLITE_ERROR (errcode 5, "database is locked"), the code userAction already treats as a storage error. This regression hits a real locked database from a second connection and checks that nothing is saved and the same action can be resent; it fails if the check is removed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate findings remain in runner/store.ts, runner/merge.ts, and github/merge.ts.

Review effort: Lite
Findings: 1

Open (1)

Closed tasks remain editable through plan writes · New

Resolved since last review (3)

Idempotency key cleared before attempt reaches terminal state
SQLite storage errors are misclassified as refusals
Swallowed outcome write leaves merge attempt stuck

Copilot review of #53 (round 4): importRevision and applySuggestion could still change a merged or cancelled task, and a snapshot recorded after closing bumped its counters. Plan writes now refuse a closed task. HEAD observation (recordHistory) still records the snapshot, because the review screen reads it after a merge, but no longer bumps a closed task's state version or context generation. Regression fails without either guard. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot

Copilot review overview

🔵 Needs a closer look

Unresolved moderate issues affect merge replay, durable failure handling, closed-task feedback, shutdown replay, and post-action status.

Review effort: Lite
Findings: None

Resolved since last review (1)

Closed tasks remain editable through plan writes

Previously missed (1)

In code that hasn't changed since last review

Map replay storage failures to retryable 503 responses

runner/merge.ts:153

#replay performs a savedAction database read before returning, but storage failures from that read are allowed to escape as ordinary errors. The server then answers 409, and the browser treats that as a definite response and clears the key, even though the action may already have been applied and the read was only transient. Map storage failures on both replay paths to MergeNotApplied/HTTP 503 so the client retains and resends the key.

Copilot review of #53 (round 5, "previously missed"): a storage failure while reading a click's saved outcome escaped #replay as an ordinary error, so the server answered 409 and the browser dropped its key although the click may already have applied. Both replay paths now map SQLite storage errors to MergeNotApplied (503); saved refusals and reused keys still pass through as definite answers. Regression fails without the mapping. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot

Copilot review overview

🔵 Needs a closer look

Seven unresolved moderate findings affect merge replay reliability and feedback-event integrity.

Review effort: Lite
Findings: None

mchwang mentioned this pull request Sep 26, 2026

F1: runner lifecycle and state-holder contract (for review) #49

Merged

mchwang and others added 2 commits September 26, 2026 01:19

Complete only into a running task (contract round 35)

4c83e78

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

mchwang force-pushed the feat/f1-store-lifecycle branch from 3ce52eb to 4c83e78 Compare September 26, 2026 08:21

mchwang changed the base branch from docs/f1-runner-lifecycle-contract to main September 26, 2026 08:21

This was referenced Sep 26, 2026

F1b: runner coordinator for attempts, slots and shutdown #56

Draft

F1c: shutdown wiring and /api/runner #57

Draft

This was referenced Sep 26, 2026

Reconcile the design plan with the code #52

Merged

F1d: startup recovery and the single-runner lock #59

Draft

F1e: planning API for lane G and feedback from review actions #60

Draft

mchwang and others added 17 commits September 27, 2026 01:14

Merge remote-tracking branch 'origin/main' into feat/f1-store-lifecycle

b960f44

mchwang and others added 3 commits September 27, 2026 05:40

mchwang marked this pull request as ready for review September 27, 2026 13:21

Copilot AI lite review requested due to automatic review settings September 27, 2026 13:21

Copilot started reviewing on behalf of mchwang September 27, 2026 13:21 View session